Add AmneziaWG cable driver - #4097
Conversation
|
🤖 Created branch: z_pr4097/sanek9/feat/amneziawg-cable-driver |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an AmneziaWG cable driver with userspace device management, configurable obfuscation, peer lifecycle handling, connection monitoring, driver registration, dependency updates, and mocked lifecycle tests. ChangesAmneziaWG cable driver
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LocalEndpoint
participant amneziawgDriver
participant startUserspaceDevice
participant AWGClient
participant Netlink
LocalEndpoint->>amneziawgDriver: provide endpoint specification
amneziawgDriver->>startUserspaceDevice: create userspace interface
amneziawgDriver->>AWGClient: configure device
amneziawgDriver->>LocalEndpoint: store device public key
amneziawgDriver->>Netlink: activate interface
amneziawgDriver->>AWGClient: configure peer
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Part of the proposal overview: #4101 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
pkg/cable/amneziawg/obfuscation.go (1)
28-30: 🩺 Stability & Availability | 🔵 TrivialCross-cluster consistency risk for
H1-H4/S1-S4via per-gateway env overrides.The comment correctly notes
H1-H4/S1-S4must be identical on every peer, butapplyCableDriverObfuscationsources overrides from each gateway's ownSUBMARINER_CABLEDRIVEROPTIONSindependently. In a multi-cluster mesh, if different clusters/gateways set different override values, handshakes will silently fail (or behave inconsistently) across the connection with no explicit cross-cluster validation or error surfaced. Worth documenting this operational requirement prominently for operators (e.g. in the CRD/API docs forcableDriverOptions), since a mismatch here is only discoverable at runtime as connectivity failures.Also applies to: 79-81
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cable/amneziawg/obfuscation.go` around lines 28 - 30, Document in the CRD/API documentation for cableDriverOptions that H1-H4 and S1-S4 overrides must be identical across every gateway and cluster in the mesh. Clearly distinguish these shared parameters from Jc/I* values, which may differ, and warn that inconsistent per-gateway SUBMARINER_CABLEDRIVEROPTIONS values cause runtime connectivity failures.pkg/cable/amneziawg/amneziawg_test.go (2)
162-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPre-populated fake peer has no effect and isn't verified.
The stale peer seeded on
t.client.devices[amneziawg.DefaultDeviceName](lines 163-165) is wiped out byConfigureDevice'sReplacePeers: truehandling (lines 354-356 in this same file) before theItblock runs, anddevice.Peersis never asserted. This setup currently does nothing and could mislead a reader into thinkingReplacePeersbehavior is exercised here.Either drop the dead setup or assert on it to get real coverage of the replace-peers path.
Suggested fix
It("should configure the device with obfuscation parameters and public key", func() { device := t.client.devices[amneziawg.DefaultDeviceName] Expect(device).ToNot(BeNil()) Expect(device.ListenPort).To(Equal(listenPort)) Expect(device.PublicKey).ToNot(Equal(wgtypes.Key{})) Expect(device.PrivateKey).ToNot(Equal(wgtypes.Key{})) Expect(device.Jc).To(Equal(7)) Expect(device.H1).To(Equal("200000000-280000000")) Expect(device.S3).To(Equal(35)) Expect(device.I1).To(Equal("<r 40>")) + Expect(device.Peers).To(BeEmpty(), "ReplacePeers should have cleared the stale peer")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cable/amneziawg/amneziawg_test.go` around lines 162 - 181, Remove the pre-populated peer setup from the BeforeEach block, or update the test to explicitly assert that ConfigureDevice replaces the seeded peer. Ensure the test meaningfully covers ReplacePeers behavior rather than retaining an unused device.Peers fixture.
94-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnused error-injection scaffolding leaves
NewDrivererror paths untested.
checkNewDriverErr(fields at lines 100, default no-op at lines 142-144) andfakeClient.configureDeviceErr(line 315) are never overridden by any test, so branches such as the PSK validation failure (w.spec.PSK == "" || w.spec.PSK == "default psk") andConfigureDevicefailure propagation inNewDriver(per driver.go snippet) go untested.Consider adding a negative-path test using these hooks, or remove the scaffolding if it's not intended to be exercised.
Also applies to: 142-144, 313-317
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cable/amneziawg/amneziawg_test.go` around lines 94 - 101, Remove the unused NewDriver error-injection scaffolding, including testDriver.checkNewDriverErr and fakeClient.configureDeviceErr, unless you add negative-path tests that override these hooks and assert PSK validation and ConfigureDevice failures propagate through NewDriver. Ensure the resulting tests explicitly cover those error branches if retaining the hooks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/cable/amneziawg/driver.go`:
- Around line 91-172: Register failure cleanup immediately after setupDevice
succeeds, before NewClient can fail. Extend the existing NewDriver error defer
to close w.userspaceDevice and delete w.link when err != nil, while preserving
client cleanup and nil assignment. Ensure all subsequent initialization failures
reclaim the userspace device, its goroutine/socket resources, and the netlink
link.
In `@pkg/cable/amneziawg/obfuscation.go`:
- Around line 83-132: Extend obfuscationFromOptions and setStringOption to
validate string options before assigning them: enforce the supported N-M syntax
for h1-h4 and AWG mini-syntax for i1-i5, warning and retaining defaults for
invalid values. After applying integer overrides, validate that Jmin is not
greater than Jmax; warn and ignore the invalid override(s), preserving a valid
fallback configuration.
---
Nitpick comments:
In `@pkg/cable/amneziawg/amneziawg_test.go`:
- Around line 162-181: Remove the pre-populated peer setup from the BeforeEach
block, or update the test to explicitly assert that ConfigureDevice replaces the
seeded peer. Ensure the test meaningfully covers ReplacePeers behavior rather
than retaining an unused device.Peers fixture.
- Around line 94-101: Remove the unused NewDriver error-injection scaffolding,
including testDriver.checkNewDriverErr and fakeClient.configureDeviceErr, unless
you add negative-path tests that override these hooks and assert PSK validation
and ConfigureDevice failures propagate through NewDriver. Ensure the resulting
tests explicitly cover those error branches if retaining the hooks.
In `@pkg/cable/amneziawg/obfuscation.go`:
- Around line 28-30: Document in the CRD/API documentation for
cableDriverOptions that H1-H4 and S1-S4 overrides must be identical across every
gateway and cluster in the mesh. Clearly distinguish these shared parameters
from Jc/I* values, which may differ, and warn that inconsistent per-gateway
SUBMARINER_CABLEDRIVEROPTIONS values cause runtime connectivity failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9fc8758a-4d17-49ec-9307-c6c6a904b75d
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
go.modpkg/cable/amneziawg/amneziawg_test.gopkg/cable/amneziawg/client.gopkg/cable/amneziawg/daemon.gopkg/cable/amneziawg/driver.gopkg/cable/amneziawg/getconnections.gopkg/cable/amneziawg/obfuscation.gopkg/cable/options.gopkg/cable/options_test.gopkg/cableengine/cableengine.go
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
pkg/cable/amneziawg/driver.go (2)
242-242: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDiscarded
removePeererrors leave no diagnostic trail.Both call sites (
ConnectToEndpointon peer-key change,DisconnectFromEndpoint) discard theremovePeererror via_ =without even a warning log. If peer removal genuinely fails (e.g., device not reachable), there's no signal of a stale peer remaining configured.Also applies to: 317-317
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cable/amneziawg/driver.go` at line 242, Update the removePeer call sites in ConnectToEndpoint and DisconnectFromEndpoint to handle removal errors instead of discarding them. Log a warning containing the error and enough peer context to diagnose a potentially stale configured peer, while preserving the existing control flow.
336-341: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
setupDevicetreats anyLinkByNameerror as "no existing link", unlikecleanupDevice.
cleanupDevice(lines 461-468) correctly distinguishesIsLinkNotFoundErrorfrom a genuine lookup failure, butsetupDevicehere silently proceeds on any non-nil error fromLinkByName, including permission/netlink errors unrelated to "not found". This could mask a real lookup problem instead of surfacing it.🛠️ Proposed fix
- if link, err := w.netLink.LinkByName(DefaultDeviceName); err == nil { - if err := w.netLink.LinkDel(link); err != nil { - return errors.Wrap(err, "failed to delete existing AmneziaWG device") - } - } + link, err := w.netLink.LinkByName(DefaultDeviceName) + switch { + case err == nil: + if err := w.netLink.LinkDel(link); err != nil { + return errors.Wrap(err, "failed to delete existing AmneziaWG device") + } + case !netlinkAPI.IsLinkNotFoundError(err): + return errors.Wrapf(err, "error checking for existing AmneziaWG device %q", DefaultDeviceName) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cable/amneziawg/driver.go` around lines 336 - 341, Update setupDevice’s LinkByName error handling to ignore only IsLinkNotFoundError, matching cleanupDevice; return or wrap any other lookup error, while preserving deletion and existing delete-error handling when the link is found.pkg/cable/amneziawg/amneziawg_test.go (1)
319-397: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFake client only tracks a subset of AmneziaWG obfuscation fields.
ConfigureDevicecopies/tracks onlyJc,S3,H1,I1(plus key/peer fields). Per the PR's documented option set (jc,jmin,jmax,s1-s4,h1-h4,i1-i5), the remaining fields are never captured, sotestNewDriver's default/override assertions can't catch regressions in those defaults.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cable/amneziawg/amneziawg_test.go` around lines 319 - 397, Extend fakeClient.ConfigureDevice to copy every AmneziaWG obfuscation field from wgtypes.Config into the stored device, including Jmin, Jmax, S1-S4, H1-H4, and I1-I5 alongside the existing fields. Preserve the current handling of keys, listen port, peers, and existing obfuscation values so testNewDriver can verify both defaults and overrides for the complete option set.pkg/cable/amneziawg/getconnections.go (1)
61-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCompare keys directly in
connectionByKey.wgtypes.Keyis comparable, so==avoids repeated string conversions in the loop.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cable/amneziawg/getconnections.go` around lines 61 - 70, Update amneziawgDriver.connectionByKey to compare the input key directly with the key returned by keyFromSpec, using wgtypes.Key equality instead of converting both keys to strings. Preserve the existing iteration and nil return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/cable/amneziawg/daemon.go`:
- Around line 75-90: Close fileUAPI in the UAPIListen error branch before
returning. Update the failure handling around ipc.UAPIListen to release the
descriptor alongside tdev and dev, while preserving the existing wrapped error
return.
In `@pkg/cable/amneziawg/driver.go`:
- Around line 285-292: The connection flow after verifyNewPeer should not record
a successful connection when verification fails. Update the error branch
handling around verifyNewPeer to return or propagate the error before reaching
the success log and cable.RecordConnection call, preserving the existing
connected path only for successful verification.
---
Nitpick comments:
In `@pkg/cable/amneziawg/amneziawg_test.go`:
- Around line 319-397: Extend fakeClient.ConfigureDevice to copy every AmneziaWG
obfuscation field from wgtypes.Config into the stored device, including Jmin,
Jmax, S1-S4, H1-H4, and I1-I5 alongside the existing fields. Preserve the
current handling of keys, listen port, peers, and existing obfuscation values so
testNewDriver can verify both defaults and overrides for the complete option
set.
In `@pkg/cable/amneziawg/driver.go`:
- Line 242: Update the removePeer call sites in ConnectToEndpoint and
DisconnectFromEndpoint to handle removal errors instead of discarding them. Log
a warning containing the error and enough peer context to diagnose a potentially
stale configured peer, while preserving the existing control flow.
- Around line 336-341: Update setupDevice’s LinkByName error handling to ignore
only IsLinkNotFoundError, matching cleanupDevice; return or wrap any other
lookup error, while preserving deletion and existing delete-error handling when
the link is found.
In `@pkg/cable/amneziawg/getconnections.go`:
- Around line 61-70: Update amneziawgDriver.connectionByKey to compare the input
key directly with the key returned by keyFromSpec, using wgtypes.Key equality
instead of converting both keys to strings. Preserve the existing iteration and
nil return behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5608666e-e67a-4a9e-8052-8d7e705325ae
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
go.modpkg/cable/amneziawg/amneziawg_test.gopkg/cable/amneziawg/client.gopkg/cable/amneziawg/daemon.gopkg/cable/amneziawg/driver.gopkg/cable/amneziawg/getconnections.gopkg/cable/amneziawg/obfuscation.gopkg/cable/amneziawg/obfuscation_test.gopkg/cable/options.gopkg/cable/options_test.gopkg/cableengine/cableengine.go
77787aa to
70af8b7
Compare
70af8b7 to
4945ece
Compare
4945ece to
e7fc459
Compare
|
@tpantelis
Separately, I also reworked option parsing for stricter validation, and so that all configuration errors are reported together instead of one at a time. Sorry if I misunderstood... after squashing and force-pushing, it’s hard to see what specifically changed for the review comments. |
e7fc459 to
d46bc5e
Compare
Actually I can see the changes via the Compare link provided by GitHub after each push. |
|
@sanek9 Would it be feasible to add the new cable driver to the CI E2E test matrix? |
c3592c1 to
b7210fc
Compare
|
@tpantelis Man, these AI tools are so smart... My employer has been paying for my Cursor subscription for half a year now, and I only started using it a week ago. All I really needed here was an AmneziaWG tunnel... and guess what? It found some repository, created another pull request, and didn't even ask me. Then it told me it wouldn't work without it and that we need to get it merged. It even wanted to leave a comment here on its own. You just give it an idea, and it runs the lab, writes the report, and then generates the code with tests. I really hope the code quality is good, though... I learned Go about 7 years ago, but I haven't had to write much in it since... |
Add an AmneziaWG-based cable driver with obfuscation options, stricter multi-error option validation, and connection status handling aligned with the WireGuard driver. Also add amneziawg to the CI E2E cable-driver matrix. Signed-off-by: sanek9 <sanya0996@gmail.com>
b7210fc to
3575b46
Compare
So all of the code changes you submitted were AI-generated... for the most part it's fine. I haven't actually deployed and run the new cable driver but the E2E job will do that. There's quite a bit of code duplicated from the wireguard cable driver and there's probably some opportunities for reuse but not a big deal. |
|
🤖 Closed branches: [z_pr4097/sanek9/feat/amneziawg-cable-driver] |
dfarrell07
added
the
release-note-needed
4 participants
What this PR does / why we need it
Adds an AmneziaWG cable driver (WireGuard-compatible with DPI obfuscation) for environments where plain WireGuard is blocked or fingerprintable.
Also makes AmneziaWG obfuscation parameters (and peer keepalive) configurable via the gateway environment variable
SUBMARINER_CABLEDRIVEROPTIONS(JSON map). When unset/empty, built-in defaults are used so older operators remain compatible.More details
pkg/cable/amneziawgregistered in the cable engine (same pattern as WireGuard).amneziawg-gouserspace stack(linked into
submariner-gateway; no separate binary and no AmneziaWG kernel module on nodes).MIT is compatible with this Apache-2.0 project.
privileged+NET_ADMIN(and host networkpath as today), which is enough to create a TUN device — no extra RBAC/securityContext changes.
jc,jmin,jmax,s1–s4,h1–h4,i1–i5.keepalive(seconds; default10;0disables).single parse are returned together so they can be fixed in one pass.
H1–H4/S1–S4must match across peers in the mesh;Jc/I*may differ per gateway.spec.cableDriverOptionsand--cable-driver-optionto populate the env var.Related issues
Checklist
Summary by CodeRabbit